Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multiple models support #54

Open
wants to merge 39 commits into
base: master
Choose a base branch
from

Conversation

kenzo-tanaka
Copy link

@kenzo-tanaka kenzo-tanaka commented Sep 18, 2022

Related: #44

This change will support multiple models.

What I fixed

  • Accept multiple model setting by klass_names
  • Display multiple login forms
  • Pass class name by params[:as]
  • In controller, find model name by params[:as] and login.

Configuration

config.klass_names = ['User', 'Staff']

Demo

any_login_multiple.mov

I checked this behavior at this repository.
kenzo-tanaka/rails_sandbox#8

@kenzo-tanaka kenzo-tanaka marked this pull request as draft September 19, 2022 04:02
Comment on lines -92 to +93
#any_login span#anylogin_back_to_user {
#any_login span.anylogin_back_to_user {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed css id to class, because some login forms would be displayed.

Comment on lines +44 to +46
def klass
params[:as].constantize
end
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This parameter is the login class (ex. User, Staff)


<span id="any_login_box">
<% AnyLogin.klasses.each do |klass| %>
<%= form_tag any_login.sign_in_path(as: klass.to_s), :method => :post, :class => 'any_login_form' do %>
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Set the login class here.

@kenzo-tanaka kenzo-tanaka marked this pull request as ready for review September 19, 2022 04:47
@kenzo-tanaka kenzo-tanaka changed the title [WIP]Multi models Multiple models support Sep 19, 2022
@kenzo-tanaka
Copy link
Author

Hello @igorkasyanchuk @rbclark , Could you review this PR ?

It was difficult to resolve conflicts on #44, so I created this new PR.

@igorkasyanchuk
Copy link
Owner

@kenzo-tanaka good start, but I have a question, could you please extend at least dummy/devise app with another model? I want to try it on your branch.

@kenzo-tanaka
Copy link
Author

Thanks for the reply. I will try it.

@kenzo-tanaka
Copy link
Author

kenzo-tanaka commented Oct 19, 2022

@igorkasyanchuk
Sorry for the delay in responding.

could you please extend at least dummy/devise app with another model?

I am having trouble understanding how to proceed this.
Do I just create a new model under the test/rails_app/devise directory and fix the any_login settings?

@rbclark
Copy link
Collaborator

rbclark commented Oct 19, 2022

I am having trouble understanding how to proceed this. Do I just create a new model under the test/rails_app/devise directory and fix the any_login settings?

Correct, specifically in the test/rails_apps/devise/app/models folder. You can set it up the same way it is in your demo with a user and a staff model if you wish.

@kenzo-tanaka
Copy link
Author

Thanks for the reply.
Is it possible to use the rails or rake commands in the test/rails_app/devise directory?

I get an error when I run the following commands, is there a workaround?

$ cd test/rails_apps/devise/
$ rails db:create    
rails aborted!
NameError: uninitialized constant Bundler
/Users/tanakakenzou/Documents/personal/any_login/test/rails_apps/devise/config/application.rb:7:in `<top (required)>'
/Users/tanakakenzou/Documents/personal/any_login/test/rails_apps/devise/Rakefile:4:in `require_relative'
/Users/tanakakenzou/Documents/personal/any_login/test/rails_apps/devise/Rakefile:4:in `<top (required)>'
bin/rails:4:in `<main>'

Caused by:
LoadError: cannot load such file -- rails/commands
bin/rails:4:in `<main>'
(See full trace by running task with --trace)

@rbclark
Copy link
Collaborator

rbclark commented Oct 19, 2022

Thanks for the reply. Is it possible to use the rails or rake commands in the test/rails_app/devise directory?

I get an error when I run the following commands, is there a workaround?

You just need to preface your commands with bundle exec,

➜  devise git:(master) pwd
/Documents/any_login/test/rails_apps/devise
➜  devise git:(master) bundle exec rails db:create
Database 'db/development.sqlite3' already exists
Database 'db/test.sqlite3' already exists

@kenzo-tanaka kenzo-tanaka marked this pull request as draft October 20, 2022 12:46
@kenzo-tanaka kenzo-tanaka marked this pull request as ready for review October 20, 2022 13:49
@kenzo-tanaka
Copy link
Author

@igorkasyanchuk @rbclark
I added a new model to the devise sample app.
Please check it.

Copy link
Collaborator

@rbclark rbclark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this! I had a few questions, mainly around the klass_name and not breaking the API for existing users.

app/views/any_login/_any_login.html.erb Outdated Show resolved Hide resolved
lib/any_login.rb Outdated
Comment on lines 24 to 25
mattr_accessor :klass_name
@@klass_name = 'User'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this PR break the ability to use the old klass_name style? If so it's likely going to require all users to update their configuration which is going to be a breaking change. Thoughts on just using klass_name and checking whether it is an array or not in the code and then handling it accordingly?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR does not break klass_name style because the default value of klass_names is [@@klass_name].
I think this is simpler than klass_name, which accepts both strings and arrays.

If you want to use any_login in multiple models, you can use klass_names, and if that is not necessary, you can continue to use klass_name.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can confirm that the klass_name style is not broken in demo apps.

  • test/rails_app/authlogic
  • test/rails_app/clearance

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just did a test in the demo devise application and it seems to confirm that klass_name is currently broken. I switched the devise demo application to use klass_name = 'Staff' and I was only presented with users and not staff:

Screen.Recording.2022-12-22.at.11.53.49.AM.mov

The reason this is happening it the klass_names is always being initialized with the default value of klass_name (which is User) and it is never taking into account the users input. The easiest way to fix this would be to just use klass_name for everything and just handle it properly if klass_name is an array.

Screenshot 2022-12-22 at 11 59 37 AM

lib/any_login/providers/authlogic.rb Show resolved Hide resolved
@rbclark
Copy link
Collaborator

rbclark commented Dec 22, 2022

Thank you @kenzo-tanaka, this is super close to being ready to merge. The only remaining issue is the klass_name problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants